/azp run cpp - core

Ah, I also think we probably should update sdk/core/azure-core/test/nlohmann-json-test/*.

As @antkmsft mentioned, we should definitely keep the source and tests versions in-sync, so an update of the nlohmann json source should be together with the test update as well.
Please address this before merging, at the very least. I'll try to review the rest, tomorrow, but would like some help in making reviewing this change easier. What should we focus on?

I'd like to make sure that the existing tests run with the newer json library, to be honest. If there are any breaking changes in the nlohmann implementation, the old tests will start failing, but the new tests would have accounted for any breaking changes.

Since our implementation was built around the old implementation, we should keep the old tests for at least the first iteration of this PR.

Once the PR is in and we've run at least a single nightly build on the new implementation with the old tests, we should integrate the new tests.

Basically, we start by using the old tests to help ensure that the new json library won't break us, then switch to the new tests to ensure that if we have tests covering any new additions to the library.

while i would agree with you there are api changes in the internal impl of the tests , the old ones don't compile properly , thus i had to update.

As mentioned in the description i ran all the CI pipelines that IMHO cover most if not all of the public interface , thus the functionality out of it that we use is covered , if we want to run some coverage tools to determine dead code in the json that we don't use that i would like to do to remove unneeded functionality, only that future updates would be harder to perform as we would need to determine what we removed and where it is and get rid of them again.

We can talk about it more monday, i did not bring this up thursday as that meeting was too clogged with go lang issues :)

We can talk about it more monday, i did not bring this up thursday as that meeting was too clogged with go lang issues :)

So be it. I was hoping to avoid doing even more work to update the tests, but if the existing tests break, so be it.

Reminder, in case this got lost -

Sorry, I just found two more macros that I missed, but that should be it! - JSON_NO_IO and JSON_TEST_KEEP_MACROS.

I can now see JSON_NO_IO prefixed, but the JSON_TEST_KEEP_MACROS is still unprefixed.

Reminder, in case this got lost -

Sorry, I just found two more macros that I missed, but that should be it! - JSON_NO_IO and JSON_TEST_KEEP_MACROS.

I can now see JSON_NO_IO prefixed, but the JSON_TEST_KEEP_MACROS is still unprefixed.

done, apologies , i somehow missed that

/azp run cpp - core

Just to understand, how come we remove INCLUDE_NLOHMANN_JSON_HPP_ instead of renaming it? It's probably right, so not suggesting we do something else, but trying to understand the rationale. Does it fail to compile with it?

@ahsonkhan, similar to #5681 (comment).
More, the pattern,

#ifndef X_HPP_INCLUDED
#define X_HPP_INCLUDED

// header file contents

#endif 

it is called "include guard". It is basically the same as

#pragma once

// header file contents

It is in our guidelines for C++ to use #pragma once instead of include guards.
Formally, #pragmas are not standardized, but practically, every C++ compiler implements it.

This is maybe not so often the case with C compilers, BTW, especially maybe for some exotic embedded platforms, which is why in Embedded C repo, we were using include guards and not pragmas.

@gearama, could you please drop all the occurrences of

/*!
@brief namespace for Niels Lohmann
@see
https://github.com/nlohmann
@since version 1.0.0
*/

?

The reason is that I used doxygen to generate docs, and what it does is that it ends up attaching namespace for Niels Lohmann as a description for Azure namespace (not the Azure::Core::Json::_internal).

/azp run cpp - core